-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moviescript: add theme #7878
Moviescript: add theme #7878
Conversation
Preview changesI've detected changes to the following themes in this PR: Moviescript. You can preview these changes by following the links below: I will update this comment with the latest preview links as you push more changes to this PR. |
This theme has a hidden pattern called header-rate that doesn't seem to be used anywhere, can you confirm if it needs deleting, @henriqueiamarino ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The heading hierarchy in this theme is hard to maintain. We generally don't want the site title to be the H1, since it's preferable to have the main heading be something that tells you what each template/page is about, ie: the post title on a single post or the archive title on archive templates, instead of being the same for every single page. But this theme lacks that main heading in many of the templates like the pages template and the index
Thanks for your review, @MaggieCabrera. Sometimes, I add a pattern that some users might use, but I don't use it in my take on the theme. I never really knew if that was okay or not. If it's not a good practice, you can delete it; no problem. I am afraid I have no control over the site title as an H1. As I design in the Editor, that is how the site title is available to me; I have other current choices. Also, I can add a title for the Page template; it's no problem. I can also make post titles H1 instead of H2 for the Index. Would that be enough? Working on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to have but since it has the hidden attribute, it won't show anywhere in the UI for users! I will change that to make it available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a couple of changes to show the hidden header/footer patterns and some translation and a11y stuff. Good to go!
Moviescript is a simple blog theme that perfectly suits writers or the general public who want their sites to be formatted as movie scripts.
Demo site